Skip to content

Skip __init__ when precompiling#328

Closed
stillyslalom wants to merge 1 commit intoJuliaPy:mainfrom
stillyslalom:main
Closed

Skip __init__ when precompiling#328
stillyslalom wants to merge 1 commit intoJuliaPy:mainfrom
stillyslalom:main

Conversation

@stillyslalom
Copy link
Copy Markdown

I thought __init__() was always skipped when precompiling, but this appears to not be the case (at least on Windows). This appears to fix stillyslalom/PyThermo.jl#7, and may also fix #266, JuliaPy/PythonPlot.jl#25, and JuliaPy/PythonPlot.jl#27.

@stillyslalom
Copy link
Copy Markdown
Author

@cjdoris could you approve the CI run?

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 4, 2023

Codecov Report

Merging #328 (8ad6e50) into main (e374e25) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #328      +/-   ##
==========================================
+ Coverage   41.50%   41.52%   +0.01%     
==========================================
  Files          76       76              
  Lines        4652     4655       +3     
==========================================
+ Hits         1931     1933       +2     
- Misses       2721     2722       +1     
Impacted Files Coverage Δ
src/PythonCall.jl 78.94% <100.00%> (+0.56%) ⬆️
src/cpython/CPython.jl 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@ufechner7
Copy link
Copy Markdown

ufechner7 commented Jul 20, 2023

What is missing to merge this commit? The bug that this commit fixes is causing issues with my co-workers when they are trying to install PythonPlot...

@stillyslalom
Copy link
Copy Markdown
Author

Pinging @cjdoris for any comments on what's blocking merge. He may just be on vacation, as I don't see any GitHub activity for about a month.

@cjdoris
Copy link
Copy Markdown
Member

cjdoris commented Jul 23, 2023

Hi yeah I've been away.

My worry with this is what happens if a package Foo that depends on PythonCall has an __init__ function which does pyimport("something")? Unless Foo.__init__ is also guarded in the same way, it assumes that PythonCall.__init__ has run already, and therefore will error because it hasn't.

Or have I misunderstood and it all works fine? I'm going to experiment a bit.

@cjdoris
Copy link
Copy Markdown
Member

cjdoris commented Jul 23, 2023

Also the linked issues all look like the same issue of conda install mysteriously failing during precompilation, which I think has been reported before but not resolved. Really efforts should be concentrated on fixing that.

Edit: it's #266

@stillyslalom
Copy link
Copy Markdown
Author

I would love to figure out the underlying issue, but this band-aid PR makes this (very annoying) problem go away. Can we merge it as a minimally-intrusive stopgap?

@cjdoris
Copy link
Copy Markdown
Member

cjdoris commented Jul 26, 2023

It is not minimally intrusive, it actually very easily breaks any downstream packages in the way I suspected above.

If I make a package Foo containing just

using PythonCall
function __init__()
    pyimport("sys")
end

then precompiling Foo fails because it runs Foo.__init__() which calls pyimport("sys") which only works if PythonCall.__init__() has already run.

I don't think this general idea can work, so I'm going to close this PR. Another option may be to initialise Python not in __init__ but whenever pyimport and friends are actually called - but this would be a larger change to the codebase.

@cjdoris cjdoris closed this Jul 26, 2023
@stillyslalom
Copy link
Copy Markdown
Author

But Foo shouldn't call __init__() during precompilation in the first place - the fact that it does is a bug that will probably need to be resolved within Julia itself. PythonPlot.jl already uses the suggested escape hatch to avoid the same bug: https://github.com/JuliaPy/PythonPlot.jl/blob/63ed915e9a3abcffa24552129d00b7f1b9099a56/src/init.jl#L147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Precompilation after add fails for packages with python dependencies Fix precompile errors on Windows

3 participants